Skip to content

Conversation

@pcj
Copy link

@pcj pcj commented Nov 17, 2014

Clarify documentation to explicitly specify position() is not supported on text elements.

Clarify documentation to explicitly specify position() is not supported on text elements.
@dmethvin
Copy link
Member

Very few jQuery APIs support text nodes, since it's relatively difficult to even get a text node into a collection. What did the code look like that prompted this docs clarification?

@pcj
Copy link
Author

pcj commented Nov 18, 2014

@dmethvin The code was related to identifying the position of a word in a text selection (bounded within a <p/>) and moving an <aside/>-like object to that y-position. It failed and took some searching about the difficulty of interrogating the position of text.

I was struck by the lack of information about it in the jquery documentation, and thought it should be clearer to developers that only Elements are supported.

@dmethvin
Copy link
Member

How did you get the text node into a jQuery object to ask for its position? You'd do something akin to $(textnode).position() I guess?

The default assumption should be that jQuery operations on text nodes are not supported. They're not supported by .attr(), .css(), .offset(), .position() .addClass(), .find(), any of the event APIs, and many others. The finding/filtering APIs explicitly remove text nodes from their input sets. It would be better to find a single place to say this rather than repeating this on all of those pages.

There already is this note in the jQuery entry, maybe we can clarify things there?

When passing HTML to jQuery(), note that text nodes are not treated as DOM elements. With the exception of a few methods (such as .content()), they are generally ignored or removed.

@dmethvin
Copy link
Member

Ah, how about if we clarify that the jQuery(element) signature is not very useful for wrapping text nodes? Probably a paragraph in the "Using DOM Elements" section. I'm assuming you got a text node into a jQuery object that way.

@arthurvr
Copy link
Member

@pcj It looks like you haven't signed the CLA yet. Could you handle that?

@pcj
Copy link
Author

pcj commented Dec 23, 2014

@arthurvr CLA signed. Thx.

@AurelioDeRosa
Copy link
Member

Is this PR good to be merged? On a side note, I think @dmethvin note could be added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two spaces before the word Position.

@arthurvr
Copy link
Member

arthurvr commented Mar 8, 2015

The default assumption should be that jQuery operations on text nodes are not supported. They're not supported by .attr(), .css(), .offset(), .position() .addClass(), .find(), any of the event APIs, and many others.

Do we really need a note on all these page (as seen in this PR)? I don't think so. IMHO a paragraph on the "using dom elements" article is enough:

Ah, how about if we clarify that the jQuery(element) signature is not very useful for wrapping text nodes? Probably a paragraph in the "Using DOM Elements" section. I'm assuming you got a text node into a jQuery object that way.

If everybody agrees with that, we should open a decent ticket for that page and close this PR.

@arthurvr
Copy link
Member

If everybody agrees with that, we should open a decent ticket for that page and close this PR.

@dmethvin @AurelioDeRosa Thoughts? I don't think having an explicit note on many of our pages makes sense, if the default assumption should be it isn't supported.

@dmethvin
Copy link
Member

Agreed, @arthurvr. Let's improve the messaging on jQuery() and perhaps .contents() which is another place where text nodes can sneak in.

We never got a reply from @pcj on how the text node ended up in the collection, but if we do we could try to address that usage as well.

@arthurvr
Copy link
Member

arthurvr commented Apr 9, 2015

Agreed, @arthurvr. Let's improve the messaging on jQuery() and perhaps .contents() which is another place where text nodes can sneak in.

There we go: #709

@arthurvr arthurvr closed this Apr 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants